Skip to content

Improve detection for Node JS#215

Merged
newpavlov merged 1 commit intomasterfrom
detect
May 20, 2021
Merged

Improve detection for Node JS#215
newpavlov merged 1 commit intomasterfrom
detect

Conversation

@josephlr
Copy link
Copy Markdown
Member

@josephlr josephlr commented May 12, 2021

Fixes #214 (CC: @devdoshi can you verify that this fixes your issue)

This uses process.versions.node to see if we are in Node.js rather
than using global.self to check if we are in a Browser.

This change also makes some minor wasm_bindgen improvements:

  • Use js_sys::global() to get the global object
  • Bind the Node's require() as module.require
  • Improving error messages
  • Being more strict about JsValue types (i.e. check that they are objects rather than just making sure they are not undefined)

Signed-off-by: Joe Richey joerichey@google.com

Copy link
Copy Markdown
Member

@newpavlov newpavlov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, but let's wait for confirmation from @devdoshi before merging.

BTW you have a typo in the PR title.

newpavlov added a commit that referenced this pull request May 14, 2021
@josephlr josephlr changed the title Imporve detection for Node JS Improve detection for Node JS May 14, 2021
This uses `process.versions.node` to see if we are in Node.js rather
than using `global.self` to check if we are in a Browser.

This change also makes some minor `wasm_bindgen` improvements:
  - Use `js_sys::global()` to get the global object
  - Bind the node require as `module.require`
  - Improving error messages

Signed-off-by: Joe Richey <joerichey@google.com>
devdoshi added a commit to opticdev/optic that referenced this pull request May 18, 2021
to reproduce:
$ OPTIC__BUILD_SKIP__UI=yes task postpull
$ yarn workspace @useoptic/ui-v2 test
@devdoshi
Copy link
Copy Markdown

Thanks @josephlr I'm checking right now via https://github.com/opticdev/optic/pull/786/checks?check_run_id=2611731376#step:6:25 but I still see the issues. Perhaps I'm not setting the dependency correctly? let me know if you can't see the logs but I believe you should be able to.

@devdoshi
Copy link
Copy Markdown

Now that I look at the issue with fresh eyes, I'm using Jest on a React UI project. It's set up to run in a jsdom environment. However, the jsdom environment does not provide an implementation for global.crypto. It appears I have to do that manually in the Jest setup, but I can definitely do that. So, I think the issue still remains that detection should be improved, but in my particular situation I think I should be making my jsdom environment include a crypto implementation, since jsdom is intended to make it look like it's in a browser, so your detection would probably continue to be fooled.

@newpavlov newpavlov merged commit 120a1d7 into master May 20, 2021
@newpavlov newpavlov deleted the detect branch May 20, 2021 08:01
takumi-earth pushed a commit to earthlings-dev/getrandom that referenced this pull request Jan 27, 2026
This uses `process.versions.node` to see if we are in Node.js rather
than using `global.self` to check if we are in a Browser.

This change also makes some minor `wasm_bindgen` improvements:
  - Use `js_sys::global()` to get the global object
  - Bind the node require as `module.require`
  - Improving error messages

Signed-off-by: Joe Richey <joerichey@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

nodejs thinks it is in a browser environment

3 participants